Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set default_flow_style=None in yaml.dump calls #52957

Merged
merged 7 commits into from
May 16, 2019
Merged

Set default_flow_style=None in yaml.dump calls #52957

merged 7 commits into from
May 16, 2019
+70 −0

Conversation

Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented May 9, 2019 •
edited by garethgreenaway
Loading

What does this PR do?

In PyYaml 5.1 they changed the default_flow_style from None to False. Changing this back to None in salt since this is a breaking change to keep backwards compatibility. Here is the PR/commit that added this: yaml/pyyaml#256 yaml/pyyaml@507a464

Would appreciate any further review as I'm not sure if we want to keep backwards compatibility or update salt to ensure default_flow_style=False works properly. But I think this is the least obtrusive change for now.

What issues does this PR fix or reference?

Fixes up some tests on Fedora which has PyYaml 5.1 installed:

#52930
#52929
#52928
#52927

PyYAML 5.1:

>>> test = {'foo': 'bar'}
>>> yaml.dump(test)
'foo: bar\n'

PyYAML 3.13:

>>> test = {'food': 'bar'}
>>> yaml.dump(test)
'{food: bar}\n'

With this change it will always dump like so:

PyYAML 5.1:

>>> yaml.dump(test, default_flow_style=None)
'{food: bar}\n'

PyYAML 3.13:

>>> yaml.dump(test, default_flow_style=None)
'{food: bar}\n'

Tests written?

Yes

Commits signed with GPG?

Yes

This was referenced May 9, 2019
Closed
Closed
Closed
waynew
waynew suggested changes May 9, 2019
View reviewed changes
tests/unit/utils/test_yamldumper.py Outdated Show resolved Hide resolved
Ch3LL and others added 2 commits May 9, 2019 15:39
@Ch3LL @waynew
Update tests/unit/utils/test_yamldumper.py
bf648e5 
Co-Authored-By: Wayne Werner <waynejwerner@gmail.com>
@Ch3LL
Update docs for yamldumper test
20bc954
@Ch3LL
Copy link
Contributor Author

Ch3LL commented May 9, 2019

started a full test run:

https://jenkinsci.saltstack.com/job/2019.2.1/job/ch3ll-yaml-salt-fedora-29-py2/
https://jenkinsci.saltstack.com/job/2019.2.1/job/ch3ll-yaml-salt-fedora-29-py3/

@waynew
Copy link
Contributor

waynew commented May 9, 2019 •
edited
Loading

I bet there's a unicode issue on py3. When we're using pytest this assertion style will actually give us information 😄 Of course, right now... 😢

You're probably aware of both these options, but you could do
self.assertEqual(salt.utils.yamldumper.dump(data), '{!!python/unicode \'foo\': !!python/unicode \'bar\'}\n') for now. Or you could do

actual = salt.utils.yamldumper.dump(data)
assert actual == '{!!python/unicode \'foo\': !!python/unicode \'bar\'}\n', actual

to see what the actual text is.

@Ch3LL
Copy link
Contributor Author

Ch3LL commented May 10, 2019

i'm fine with just sticking to just assert. I know we do not get more information, but this will make it an easier transition to pytest and will get more information then.

@Ch3LL Ch3LL requested a review from a team May 10, 2019 18:57
@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #52957 into 2019.2.1 will increase coverage by 7.54%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           2019.2.1   #52957      +/-   ##
============================================
+ Coverage     28.78%   36.32%   +7.54%     
============================================
  Files          1563     1576      +13     
  Lines        267054   269470    +2416     
  Branches      57953    56051    -1902     
============================================
+ Hits          76862    97896   +21034     
+ Misses       189762   160525   -29237     
- Partials        430    11049   +10619
Flag Coverage Δ
#arch ?
#centos7 ?
#fedora29 36.32% <100%> (?)
#proxy ?
#py2 36.32% <100%> (+7.63%) ⬆️
#py3 ?
#ubuntu1604 ?
Impacted Files Coverage Δ
salt/utils/yamldumper.py 86.27% <100%> (+10.76%) ⬆️
salt/serializers/yaml.py 77.38% <100%> (+7.5%) ⬆️
salt/serializers/yamlex.py 75% <100%> (+64.71%) ⬆️
salt/transport/frame.py 27.27% <0%> (-51.52%) ⬇️
salt/ext/ssl_match_hostname.py 0% <0%> (-36.18%) ⬇️
salt/output/virt_query.py 12.9% <0%> (-35.49%) ⬇️
salt/utils/stringio.py 52.38% <0%> (-33.34%) ⬇️
salt/modules/pdbedit.py 18.33% <0%> (-31.67%) ⬇️
salt/utils/pkg/deb.py 31.57% <0%> (-31.58%) ⬇️
salt/modules/pacmanpkg.py 25% <0%> (-31.44%) ⬇️
... and 1538 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee05da5...e317186. Read the comment docs.

@codecov
Copy link

codecov bot commented May 13, 2019 •
edited
Loading

Codecov Report

Merging #52957 into 2019.2.1 will increase coverage by 6.88%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           2019.2.1   #52957      +/-   ##
============================================
+ Coverage     29.78%   36.66%   +6.88%     
============================================
  Files          1574     1577       +3     
  Lines        269397   269540     +143     
  Branches      58069    57477     -592     
============================================
+ Hits          80227    98832   +18605     
+ Misses       188741   159768   -28973     
- Partials        429    10940   +10511
Flag Coverage Δ
#arch ?
#centos7 ?
#fedora29 36.66% <100%> (?)
#proxy ?
#py2 36.32% <100%> (+6.63%) ⬆️
#py3 35.88% <100%> (+26.12%) ⬆️
#ubuntu1604 ?
Impacted Files Coverage Δ
salt/utils/yamldumper.py 86.27% <100%> (+10.76%) ⬆️
salt/serializers/yaml.py 78.57% <100%> (+8.69%) ⬆️
salt/serializers/yamlex.py 78.97% <100%> (+68.69%) ⬆️
salt/ext/ssl_match_hostname.py 0% <0%> (-36.18%) ⬇️
salt/output/virt_query.py 12.9% <0%> (-35.49%) ⬇️
salt/modules/pdbedit.py 18.33% <0%> (-31.67%) ⬇️
salt/utils/pkg/deb.py 31.57% <0%> (-31.58%) ⬇️
salt/modules/pacmanpkg.py 25% <0%> (-31.44%) ⬇️
salt/runners/bgp.py 21.48% <0%> (-31.41%) ⬇️
salt/modules/bcache.py 8.5% <0%> (-31.33%) ⬇️
... and 1518 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f410346...5f6581a. Read the comment docs.

@Ch3LL Ch3LL merged commit bd02ea6 into saltstack:2019.2.1 May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waynew waynew waynew approved these changes

@garethgreenaway garethgreenaway garethgreenaway approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ch3LL @waynew @garethgreenaway